Skip to content

Dev/fast diff#10

Open
genail wants to merge 25 commits intomasterfrom
dev/fast_diff
Open

Dev/fast diff#10
genail wants to merge 25 commits intomasterfrom
dev/fast_diff

Conversation

@genail
Copy link
Contributor

@genail genail commented Aug 14, 2024

No description provided.

@genail
Copy link
Contributor Author

genail commented Aug 14, 2024

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Key issues to review

Possible Bug
The method check_if_option_file_exists_and_readable has been modified to handle multiple files. However, the implementation might not correctly handle cases where the array flag is not set, potentially leading to unexpected behavior when splitting the value on a comma. This needs a thorough review to ensure it handles both single and multiple file paths correctly.

Performance Issue
The method process_request now includes a complex parameter manipulation which could be inefficient, especially with large arrays or deep nested structures. Consider optimizing this part of the code or simplifying the parameter handling.

Code Complexity
The Pack1Packer class is quite complex and handles many aspects of file packing, which could make it difficult to maintain and debug. Consider refactoring into smaller, more manageable components.

Code Smell
The Packer class tightly couples file operations and SHA1 hashing which might lead to issues if changes are needed independently. Consider decoupling these functionalities.

@genail
Copy link
Contributor Author

genail commented Aug 27, 2024 •
edited
Loading

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Possible bug
Handle potential nil or empty string in value before operations

Consider handling the case where value is nil or an empty string before attempting
to split it or check if it's a file. This can prevent potential errors or unexpected
behavior if value is not set as expected.

app/core/base_tool2.rb [175-179]

 if array
-  arr = value.split(',')
+  arr = value.to_s.split(',')
 else
-  arr = [value]
+  arr = [value.to_s]
 end
Suggestion importance[1-10]: 9

Why: The suggestion addresses a potential bug where value could be nil or an empty string, which would cause errors when calling split or checking if it's a file. This is a significant improvement for robustness.

9
Maintainability
Refactor file upload code into a separate method

Refactor the upload process into a separate method to reduce the complexity of the
execute method and improve code readability.

app/upload_version.rb [134-162]

 @file.each do |file|
+  upload_file(file)
+end
+
+def upload_file(file)
   file_size = File.size(file)
   progress_bar = ProgressBar.new(file_size)
   ...
   uploader.upload_file(file)
   ...
 end
Suggestion importance[1-10]: 9

Why: Refactoring the file upload process into a separate method significantly reduces the complexity of the execute method, enhancing readability and maintainability.

9
Encapsulate the retry count decrement and check into methods

Replace the direct use of @retry_count with a method that encapsulates the retry
logic. This will improve maintainability and encapsulation of the retry mechanism.

app/upload_version.rb [166-167]

-@retry_count -= 1
-if @retry_count < 0
+decrement_retry_count
+if retry_limit_reached?
Suggestion importance[1-10]: 8

Why: Encapsulating the retry logic into methods improves maintainability and readability by reducing direct manipulation of instance variables and centralizing the retry logic.

8
Best practice
Avoid modifying the original params object by using dup

To ensure that the params hash is not modified during the process, consider using
dup to create a copy of params before deleting keys. This prevents side effects on
the original params object passed to the method.

app/core/patchkit_api.rb [55-64]

-if params.include?(:params)
-  formatted_params = params.delete(:params).flat_map do |k, v|
+local_params = params.dup
+if local_params.include?(:params)
+  formatted_params = local_params.delete(:params).flat_map do |k, v|
     if v.is_a?(Array)
       v.map { |item| ["#{k}[]", item.to_s] }
     else
       [[k.to_s, v.to_s]]
     end
   end
   request.form = formatted_params
 end
Suggestion importance[1-10]: 8

Why: This suggestion follows best practices by avoiding side effects on the original params object, which can prevent unintended bugs and improve code maintainability.

8
Use refinements to inject functionality into Object to avoid global effects

To avoid potential issues with method redefinition or unexpected behavior in the
Object class, consider using refinements or module prepend to inject functionality.
This approach provides better encapsulation and avoids global monkey patching.

app/core/utils/ext.rb [21-25]

-class Object
-  include NotNil
-  include Blank
-  include NotBlank
+module EnhancedObject
+  refine Object do
+    include NotNil
+    include Blank
+    include NotBlank
+  end
 end
Suggestion importance[1-10]: 8

Why: Using refinements to inject functionality into Object is a best practice that avoids global side effects and potential conflicts, improving code safety and maintainability.

8
Replace magic string with a constant for the retry prompt

Use a constant for the retry prompt default value to avoid magic strings and improve
code clarity.

app/upload_version.rb [169]

-if ask_yes_or_no("Try again?", 'y')
+DEFAULT_RETRY_RESPONSE = 'y'
+if ask_yes_or_no("Try again?", DEFAULT_RETRY_RESPONSE)
Suggestion importance[1-10]: 6

Why: Using a constant for the retry prompt default value avoids magic strings and improves code clarity, making it easier to manage and understand the default values used in the code.

6
Performance
Use File.exist? instead of File.directory? for efficiency

To improve the efficiency of the create_diff method, consider using File.exist?
instead of File.directory? for checking the existence of directories. This is
because File.exist? checks for both files and directories and is generally faster.

app/core/patchkit_version_diff.rb [51]

-FileUtils.mkdir_p temp_dir unless File.directory?(temp_dir)
+FileUtils.mkdir_p temp_dir unless File.exist?(temp_dir)
Suggestion importance[1-10]: 7

Why: The suggestion offers a minor performance improvement by using a more efficient method to check for directory existence. While the impact may be small, it is a valid optimization.

7
Possible issue
Add error handling for the SHA1 attribute

Consider adding error handling for the new :sha1 attribute to ensure it is provided
when required and valid, similar to other critical attributes.

app/upload_version.rb [41]

 :sha1
+raise ArgumentError, "SHA1 must be provided and valid" unless valid_sha1?(@sha1)
Suggestion importance[1-10]: 7

Why: Adding error handling for the SHA1 attribute ensures that it is validated, which can prevent potential runtime errors and improve the robustness of the code.

7

genail added 19 commits August 29, 2024 10:35
@genail
Add turbopatch support, add hashes checks for diff with xxhash
f312718
@genail
Signatures downloading: fix download for files bigger than 512
7eed5c7 
It was using the wrong download method if the signatures file has been hosted with S3 signed URL instead of CDN url.
@genail
Diff processing: Fix Windows permission denied error
acebe4f 
On Windows, the @io_proxy object could be holding an open file descriptor on the target zip filename when processing a diff file. Fixed it by not opening it at all.
@genail
3.6.1 release
8bc6aa8
@genail
Enhance error handling in version.rb for signature downloads. Added a…
5af44ec 
… retry mechanism for 404 errors, allowing the process to wait and attempt to download signatures again after a minute.
@genail
Fix rubocop, change local ruby version
b51e2f0
@genail
Add run-tests.sh test runner wrapper
8c03094
@genail
Add .cursorrules
b4274b4
@genail
PatchKitVersionDiff: Allow underscored signatures
520d14a 
This handles case with windows exe and dll files problem.
@genail
Enhance ZipHelper: Add option to append underscores to extracted file…
ff6f675 
… names and implement tests for unzip functionality. This allows users to choose whether to add underscores to filenames during extraction, improving file management flexibility.
@genail
Diff version: Use underscore signatures by default
c75dde2
@genail
Add some puts for better formatting
2f5e4ef
@genail
Version bump to 3.6.2
ddb38da
@genail
Update Rakefile to include versioning in package naming convention
a9751c9 
- Added a requirement for the version file to access the current version of PatchKitTools.
- Modified the package naming in the create_package method to include the version number in the output zip file name, improving clarity and version tracking.
@genail
Fix api key inclusion
d7b2db1
@genail
Add Windows long path support (\\?\) to bypass 260-char MAX_PATH limit
c3f7b16 
Introduces WindowsLongPath module with three methods:
- fix(): adds \\?\ prefix for Ruby File I/O operations
- safe_rm_rf(): uses rd /s /q on Windows (FileUtils.rm_rf fails with \\?\)
- short_path(): returns 8.3 short names for native C FFI calls (librsync,
  turbopatch, xxhash) since fopen() cannot handle \\?\ prefix

Applied across 18 files. Design choices:
- Zip::File.open/foreach do NOT use fix() (rubyzip incompatible with \\?\)
- zip.add source paths DO use fix() (rubyzip reads via File.open internally)
- All methods are no-ops on non-Windows platforms

Also fixes: upload_version.rb app method was defined outside class scope,
platform.rb x64 CPU detection, librsync.rb absolute DLL path resolution,
and adds non-ASCII filename filtering in content packaging.
@genail
Version bump to 3.6.3
221d27c
@genail
Add retry logic for delta generation to handle AV file locking
7d82b7b 
rs_rdiff_delta() return value was previously ignored and output file
size was not validated. Antivirus software on client machines can
interfere with file I/O, producing 0-byte delta files that crash
server-side content processing with rdiff patch error 103
(RS_INPUT_ENDED).

build_delta() now retries up to 3 times with a 3-second delay,
checking both the native function return code and the output file
size (minimum 5 bytes for a valid delta).

Version bump to 3.6.4
@genail
Allow channel direct publish when API grants permission
5dc302c 
Check allow_channel_direct_publish from API response before blocking
non-diff_fast uploads to channel apps.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Review effort [1-5]: 4

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@genail